test: add static export E2E tests (#564)#686
Conversation
Add Playwright E2E tests for `output: "export"` static builds covering both App Router and Pages Router. Includes a lightweight static file server with path traversal protection, proper error handling, and startup validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/bigbonk review |
commit: |
There was a problem hiding this comment.
Review: Static Export E2E Tests
Clean PR — well-structured tests, solid static file server, and proper CI wiring. The test coverage is good for an initial static export E2E suite (both App Router and Pages Router, including dynamic routes, metadata, __NEXT_DATA__, and 404 behavior).
One issue to address in the static file server, and a few minor suggestions below.
| if (pathname.endsWith("/")) pathname += "index.html"; | ||
|
|
||
| // Resolve file path — try exact match, then .html extension | ||
| let filePath = resolve(join(rootDir, pathname)); |
There was a problem hiding this comment.
Nit: resolve(join(rootDir, pathname)) is redundant since resolve with an absolute first segment in the join result already normalizes. But pathname starts with /, so join(rootDir, pathname) actually works correctly (join strips the leading / from pathname when the first arg is absolute).
Just noting this is fine as-is.
| await expect(nav.locator('a[href="/products/widget"]')).toBeVisible(); | ||
| }); | ||
|
|
||
| test("client-side navigation works between pages", async ({ page }) => { |
There was a problem hiding this comment.
This test verifies client-side navigation works, which is great for a static export. One thing to note: since this is a full page navigation (no vinext runtime for SPA transitions in a static export), page.waitForURL will work fine. But if static export eventually supports client-side hydration with the router, this test's behavior would change. Worth a brief comment.
Also, consider verifying the navigation didn't produce a network error (checking response status on the target page), similar to other tests in this file.
| // lightweight static file server. No vinext runtime is needed — | ||
| // the output is pure pre-rendered HTML files. | ||
| command: | ||
| "npx tsc -p ../../../packages/vinext/tsconfig.json && node ../../../packages/vinext/dist/cli.js build && node ../../../tests/e2e/static-export/serve-static.mjs dist/client 4180", |
There was a problem hiding this comment.
The tsc step compiles the entire vinext package before building — same pattern as pages-router-prod. Just confirming: is this needed because the static-export fixture depends on vinext from source (workspace dependency), and the CLI invocation (dist/cli.js) needs the compiled output? If so, this is correct.
One concern: in CI, the e2e job already runs vp run build (line 146 of ci.yml) before Playwright. So npx tsc -p ../../../packages/vinext/tsconfig.json here is redundant in CI but necessary for local development (where you might not have run the build). The pages-router-prod project has the same pattern, so this is consistent.
| process.exit(1); | ||
| } | ||
|
|
||
| const MIME_TYPES = { |
There was a problem hiding this comment.
Nice set of MIME types. Consider adding .txt (text/plain) and .woff2 (font/woff2) for completeness — static exports sometimes include font files or robots.txt. Not blocking.
|
Review submitted successfully. Here's a summary of the feedback I posted: Overall: Clean PR with well-structured tests and solid static file server implementation. Good coverage across both App Router and Pages Router static export scenarios. Key feedback points:
|
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: Static Export E2E Tests
Clean, well-structured PR. The test coverage is appropriate for an initial static export E2E suite — it exercises both App Router and Pages Router, including dynamic routes with generateStaticParams/getStaticPaths, metadata, __NEXT_DATA__, and 404 behavior. The static file server is solid and purpose-built.
I verified:
- All test assertions match the fixture source content (titles, text, slugs, product names)
- The Playwright config follows the same patterns as existing projects (
pages-router-produses the sametsc + cli.js buildapproach) - The CI matrix entry is correct
- The
serve-static.mjspath traversal protection is sound (decode → resolve → prefix check) - The
BASEconstant pattern is consistent with all other E2E specs in the repo
A few minor observations below, none blocking.
| process.exit(1); | ||
| }); | ||
|
|
||
| server.listen(port, "127.0.0.1", () => { |
There was a problem hiding this comment.
Nit: The server binds to 127.0.0.1 but the log message says localhost. This is fine in practice (they resolve to the same thing), but if someone were to change this to 0.0.0.0 for debugging, the log would be misleading. Consider using the actual bind address in the log:
| server.listen(port, "127.0.0.1", () => { | |
| server.listen(port, "127.0.0.1", () => { | |
| console.log(`Static server listening on http://127.0.0.1:${port}`); | |
| }); |
Not blocking — localhost is clearer for most people.
| await expect(page).toHaveTitle("Static Export Fixture"); | ||
| }); | ||
|
|
||
| test("404 page for non-existent route", async ({ page }) => { |
There was a problem hiding this comment.
The 404 test depends on serve-static.mjs returning a 404 status for missing files. If vinext's static export generates a 404.html file, the server serves it with status 404 (line 108 of serve-static.mjs). If vinext does not generate 404.html, the server still returns 404 with a plain text body.
Either way the test passes, but the behavior being tested is subtly different. It might be worth adding a follow-up test that checks the 404 content (e.g., whether the custom 404 page is rendered) to verify vinext's static export actually produces a 404.html. That would be a more meaningful signal for issue #564 coverage.
Not blocking for this PR — just a suggestion for a follow-up.
| await expect(page.locator("body")).toContainText("Product ID: doohickey"); | ||
| }); | ||
|
|
||
| test("__NEXT_DATA__ is present in Pages Router output", async ({ page }) => { |
There was a problem hiding this comment.
Good test — verifying __NEXT_DATA__ is present and correctly shaped is exactly the right thing to check for Pages Router static export parity. This catches the case where vinext's static export might emit the HTML but forget the data script tag.
| if (pathname.endsWith("/")) pathname += "index.html"; | ||
|
|
||
| // Resolve file path — try exact match, then .html extension | ||
| let filePath = resolve(join(rootDir, pathname)); |
There was a problem hiding this comment.
Worth noting for future readers: join(rootDir, pathname) works correctly here even though pathname starts with /. Node's path.join strips the leading / from non-first arguments, so join('/srv/root', '/about') produces /srv/root/about, not /about. The subsequent resolve() then normalizes any remaining .. segments.
The two-step resolve(join(...)) + isInsideRoot() is the correct pattern — resolve handles normalization, and the prefix check catches any traversal that survives.
|
Review submitted successfully. Here's a summary of the feedback I posted: Verdict: Approved This is a clean, well-structured PR. The test coverage is appropriate for an initial static export E2E suite covering both App Router and Pages Router, including dynamic routes, metadata, What I verified:
Inline comments posted:
|
Summary
output: "export"static builds covering both App Router and Pages Routerserve-static.mjs) with path traversal protection, proper error handling, and startup validationstatic-exportproject inplaywright.config.tsand the CI matrixDetails
New files
tests/e2e/static-export/app-router.spec.ts— 10 tests covering home, about, blog slugs, navigation, metadata, and 404 handlingtests/e2e/static-export/pages-router.spec.ts— 7 tests covering Pages Router pages,getStaticProps/getStaticPathsdata,__NEXT_DATA__verification, and 404 for non-pre-rendered routestests/e2e/static-export/serve-static.mjs— Zero-dependency static file server with:server.on("error")handler for EADDRINUSEModified files
playwright.config.ts— Newstatic-exportproject entry (port 4180, 60s timeout).github/workflows/ci.yml— Addstatic-exportto E2E matrixTest plan
vp check(formatting + types) passes